-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Quantization descriptions more coherent #68
Conversation
ffv1.md
Outdated
|
||
There are 5 quantization tables for the 5 sample differences, both the number of quantization steps and their distribution are stored in the bitstream. Each quantization table has exactly 256 entries, and the 8 least significant bits of the sample difference are used as index: | ||
Each quantization table contains 5 blocks, each block corresponding to 1 of the 5 sample differences, both the number of quantization steps and their distribution are stored in the bitstream. Each quantization table block has exactly 256 entries, and the 8 least significant bits of the sample difference are used as index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps clearer language. For instance a table of 10 blocks does contain 5 blocks
. Could you say contains exactly 5 blocks
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a titlecase, backtick-quoted term Quantized Sample Differences
should be used both in this section and in the prior Context section to clarify that these are the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to prior comment the concept of blocks
and quantization table block
above could be a more formal term such as:
Each
Quantization Table
contains 5Quantization Table Blocks
with each block corresponding to 1 of the 5Quantized Sample Differences
. Both the number of quantization steps and their distribution are stored in the bitstream. EachQuantization Table Block
has exactly 256 entries, and the 8 least significant bits of theQuantized Sample Difference
used as index:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding each block corresponding to 1 of the 5 sample differences
, is an order implied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding are stored in the bitstream
, could this be more specific as to where this is stored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except each block corresponding to 1 of the 5 sample differences
comments (IMO order is explicated with Q_1
, Q_2
... And will be more explicit with the next PR changing them in an array), all comments are included in a the new version.
ffv1.md
Outdated
RFC:``` | ||
|
||
In this formula, `i` is the quantization table index of the currently parsed data, `j` is the sample difference index, `k` the sample difference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly, that the =
would be read as is assigned
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right (see "Assignment operators" part)
ffv1.md
Outdated
|
||
## Quantization table indexes | ||
|
||
Several quantization tables can be used in the same bitstream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sentence needed? It just seems to say that the bitstream (ffv1 bitstream, I suppose) can contain more than 1 quantization table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
ffv1.md
Outdated
@@ -797,7 +810,7 @@ Inferred to be 1 if not present. | |||
### quant_table_index | |||
`quant_table_index` indicates the index to select the quantization table set and the initial states for the slice. | |||
`quant_table_index` indicates the index to select the quantization table and the initial states for the slice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Quantization table indexes
is designed earlier, I think the term should be used in the definition here.
I added another commit so changes compared to remarks are visible, please review then I can squash them. |
ffv1.md
Outdated
|
||
There are 5 quantization tables for the 5 sample differences, both the number of quantization steps and their distribution are stored in the bitstream. Each quantization table has exactly 256 entries, and the 8 least significant bits of the sample difference are used as index: | ||
Each Quantization Table contains 5 exactly Quantization Table Blocks, each block corresponding to 1 of the 5 Quantized Sample Differences, both the number of quantization steps and their distribution [are stored in the bitstream](#quantization-table). Each Quantization Table Block has exactly 256 entries, and the 8 least significant bits of the Quantized Sample Differences are used as index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean exactly 5
rather than 5 exactly
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the intent of and their distribution [are stored in the bitstream](#quantization-table)
. The bracketed label does not seem to match the section link.
ffv1.md
Outdated
|
||
## Quantization Table indexes | ||
|
||
For each plane of each slice, a Quantization Table is selected from an index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may need an extra line break after line 249 for the bulleted list to render correctly in the RFC outputs.
ffv1.md
Outdated
@@ -160,7 +160,7 @@ NumBytes is a non-negative integer that expresses the size in 8-bit octets of pa | |||
|
|||
# General Description | |||
|
|||
Samples within a plane are coded in raster scan order (left->right, top->bottom). Each sample is predicted by the median predictor from samples in the same plane and the difference is stored see [Coding of the Sample Difference](#coding-of-the-sample-difference). | |||
Samples within a plane are coded in raster scan order (left->right, top->bottom). Each sample is predicted by the median predictor from samples in the same plane and the difference is stored see [Coding of the Quantized Sample Differences](#coding-of-the-quantized-sample-difference). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mismatch in the section link.
coding-of-the-quantized-sample-difference
vs
Coding of the Quantized Sample Differences
I suggest adding an s
to the end of the link.
@dericed I added another update, please review. I did not change something for:
as I don't see what you expect: you wanted to know where the quantization tables are stored, are linked to the corresponding place ("Quantization table" section in the bitstream part of the specifications). |
c124358
to
4e412b7
Compare
Commits are combined now, please review again and confirm all is fine. |
4e412b7
to
076993b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the extent that I understand the content, LGTM.
ffv1.md
Outdated
@@ -160,7 +160,7 @@ NumBytes is a non-negative integer that expresses the size in 8-bit octets of pa | |||
|
|||
# General Description | |||
|
|||
Samples within a plane are coded in raster scan order (left->right, top->bottom). Each sample is predicted by the median predictor from samples in the same plane and the difference is stored see [Coding of the Sample Difference](#coding-of-the-sample-difference). | |||
Samples within a plane are coded in raster scan order (left->right, top->bottom). Each sample is predicted by the median predictor from samples in the same plane and the difference is stored see [Coding of the Quantized Sample Differences](#coding-of-the-quantized-sample-differences). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample difference is coded not the quantized sample difference.
ffv1.md
Outdated
|
||
There are 5 quantization tables for the 5 sample differences, both the number of quantization steps and their distribution are stored in the bitstream. Each quantization table has exactly 256 entries, and the 8 least significant bits of the sample difference are used as index: | ||
Each Quantization Table contains exactly 5 Quantization Table Blocks, each block corresponding to 1 of the 5 Quantized Sample Differences, both the number of quantization steps and their distribution are stored in the bitstream. Each Quantization Table Block has exactly 256 entries, and the 8 least significant bits of the Quantized Sample Differences are used as index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Table in the most general sense maps an input parameter to an output parameter.
Before this change each of the 5 tables mapped a bounded scalar integer to a scalar integer of smaller range. Aka performing scalar quantization.
What does the single table do afterwards ? Its input has to be a 5 dimensional vector to fill the same role. While this works it is not how it can be implemented. I think the text was clearer before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I also prefer the original use of the term 'table' as a map rather than introduce the new concept of 'table block' which is a term I have less understanding of. Relatedly I see ISO-14495 uses the term 'mapping table' and 'mapping table entries'. Is it worthwhile sharing this terminology?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current spec uses at one place "quantization table set", please comment about using this terminology everywhere (it was a request in my PR introduction).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with "quantization table set" but it should be defined and probably reused in the line 238 we're discussion above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelni and @dericed are you both OK for using the alternative form proposed in the intro of my PR ("quantization tables sets", "quantization table set", "quantization table"), meaning that there will be lot of changes a bit everywhere in the spec ("quantization table" must be change nearly everywhere by "quantization table set").
I don't want to spend time on that if the idea is not agreed by everyone first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see above you had:
At the beginning, I tried to do with:
"quantization tables sets", having quant_table_set_count "quantization table set"
"quantization table set", having 5 "quantization table"
"quantization table", having 256 entries
but it was more changes in the document, and I preferred to avoid so many changes, is it > OK?
I don't have a strong opinion, but to me 'quantization table set' and 'quantization table' make sense and I suggest in addition to putting these in backtick quotes and title case that they be defined as local terminology as well.
To me 'quantization tables sets' is less elegant, particularly since it contains data that is not only 'quantization tables set'.
I suggest the 'entries' mentioned above be a defined term as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping, I wish to know in which direction you want me to go in order to get the PR accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion, but to me 'quantization table set' and 'quantization table' make sense [..]
To me 'quantization tables sets' is less elegant, particularly since it contains data that is not only 'quantization tables set'.
+1
Cut-off date for draft updates for IETF99 is July 3rd. Any chance we can resolve the issues of this pull request before then. |
I plan to work again on it tomorrow and send a new proposal, then it will depends on comments, I guess. |
6e6de94
to
f89368b
Compare
@dericed and @michaelni: Please review again. |
ffv1.md
Outdated
@@ -1227,11 +1242,11 @@ QuantizationTablePerContext(i, j, scale) { | | |||
### quant_tables | |||
`quant_tables` indicates the quantification table values. | |||
`quant_tables[ i ][ j ][ k ]` indicates the quantification table value of the Quantized Sample Difference `k` of the Quantization Table `j` of the Set Quantization Table `i`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this end with?
of the Quantization Table Set
i
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this end with?
of the Quantization Table Set
i
.
Fixed.
RFC:``` | ||
|
||
In this formula, `i` is the Quantization Table Set index, `j` is the Quantized Table index, `k` the Quantized Sample Difference. | ||
|
||
## Quantization Table Set indexes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick but I would suggest using the singular index
in the section header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is a general section about all Quantization Table Set indexes (there are several indexes in each slice, ~1 per plane), I am in favor of keeping plural without being against singular, waiting for another person for suggestion.
Two minor comments, but otherwise this looks good. Thanks. |
"Quantization Table" becomes "Quantization Table Set" "Quantization Table Per Context" becomes "Quantization Table" And some rewording in order to be more explicit
f89368b
to
4aaf7c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
This requires to be re checked, there are issues in it, for example quant_tables is interpreted as quant subscript t. There is also trailing whitespace, The "the Template for Context and Median Prediction" link also doesnt work, thats seems to be an older issue though |
The "Restrictions" section seems to have "Quantization Table Set" as parent in the pdf at least |
The trailing whitespaces I added are for carriage return when converted to HTML (markdown considers single carriage returns without white space as no carriage return), was from my point of view easier to read. I suggest to keep them, without being against removing them. I push soon another PR for the other issues. |
I did not realize that, thx for explaining |
I don't understand this sentence. |
In the pdf output some quant_tables are written with a small t, as if t was a subscript |
I understand that there is some incoherencies in the description of Quantization related items, i.e. "context" is used with 2 different meanings, the are in some places "quantization table set" never defined and in other places "quantization table" which is similar the "quantization table set".
I tried to have something more coherent, with 3 items:
At the beginning, I tried to do with:
but it was more changes in the document, and I preferred to avoid so many changes, is it OK?
Other changes:
a − b
replaced byk
, asa − b
detail is not needed at this place (andk
is chosen because it is already used in QuantizationTable() definition)set
removed when present, in order to have a single item for the same thingTable_
is removed because it is never defined and same as quant_tables[i]Quantization table index
is described (with more background about the exception)QuantizationTablePerContext
changed toQuantizationTablePerBlock
becauseContext
is also used (see Context paragraph)Added missing space in the list of negative numbers